Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MecanumDriveOdometry to handle odometry estimation of Mecanum wheeled models #486

Merged
merged 21 commits into from
Aug 22, 2023

Conversation

danilogsch
Copy link
Contributor

@danilogsch danilogsch commented Aug 25, 2022

🎉 New feature

Closes #683

Summary

Math Class to handle odometry estimation of Mecanum wheeled models.
Observation: Aproximation when the robot is given both linear, lateral and angular velocity commands. Exact pose integration or Runge Kuffa second order integration happens if one the velocity straight commands (linear or lateral) is 0.

Test it

run mecanum_drive.sdf and listen the odometry and tf topics.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check

We are also adding python interfaces to new classes, if you don't have time to do it, please open an issue about adding python interfaces.

include/ignition/math/MecanumDriveOdometry.hh Outdated Show resolved Hide resolved
include/ignition/math/MecanumDriveOdometry.hh Outdated Show resolved Hide resolved
include/ignition/math/MecanumDriveOdometry.hh Outdated Show resolved Hide resolved
src/MecanumDriveOdometry.cc Outdated Show resolved Hide resolved
src/MecanumDriveOdometry.cc Outdated Show resolved Hide resolved
@danilogsch
Copy link
Contributor Author

depends on #1664 of gz-sim

danilogsch and others added 7 commits August 25, 2022 13:56
…heels odometry

Signed-off-by: danilo_gsch <danilo_gsch@hotmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: danilo_gsch <danilo_gsch@hotmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: danilo_gsch <danilo_gsch@hotmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: danilo_gsch <danilo_gsch@hotmail.com>
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: danilo_gsch <danilo_gsch@hotmail.com>
Signed-off-by: danilo_gsch <danilo_gsch@hotmail.com>
Signed-off-by: danilo_gsch <danilo_gsch@hotmail.com>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

linters https://github.com/gazebosim/gz-math/actions/runs/3534393561/jobs/5931205749

  /github/workspace/include/ignition/math/MecanumDriveOdometry.hh:81:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:36:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
  /github/workspace/src/MecanumDriveOdometry.cc:41:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:144:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:152:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:153:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:154:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:155:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:176:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:177:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:180:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:181:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:182:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:206:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:272:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:285:  Missing username in TODO; it should look like "// TODO(my_username): Stuff."  [readability/todo] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:286:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  /github/workspace/src/MecanumDriveOdometry.cc:287:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
  Total errors found: 18

@ahcorde ahcorde changed the title Class ignition::math::MecanumDriveOdometryPrivate to handle Mecanum w… MecanumDriveOdometry to handle odometry estimation of Mecanum wheeled models Nov 24, 2022
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@azeey
Copy link
Contributor

azeey commented Feb 13, 2023

@danilogsch Do you think you will have time to fix the linter issues and address @ahcorde's feedback ?

@mjcarroll
Copy link
Contributor

@danilogsch friendly ping

@danilogsch
Copy link
Contributor Author

hey, sorry that I haven't reached here in a while... Is there anything else you need me to do here?

@Bi0T1N
Copy link
Contributor

Bi0T1N commented May 6, 2023

Looks like linter errors are resolved, only Ubuntu Bionic lists

/github/workspace/include/ignition/math/MecanumDriveOdometry.hh:81:  Line ends in whitespace.  Consider deleting these extra spaces.  [whitespace/end_of_line] [4]
/github/workspace/include/ignition/math/MecanumDriveOdometry.hh:81:  Lines should be <= 80 characters long  [whitespace/line_length] [2]

but this could probably be neglected since Focal/Jammy don't complain.

However, you have to add tests - probably similar to the ones in DiffDriveOdometry_TEST.cc.
Also your last commit isn't signed but one option could be to amend, sign and force-pushing the new commit to your branch.

@danilogsch
Copy link
Contributor Author

@Bi0T1N , thanks for the insight... I had no idea on how to deal with unsigned commits.
Anyway, I have time this week and I will work on the tests.

@muttistefano
Copy link
Contributor

Hi, is there anything I can do to help here?
I have been waiting for this to be merged for a while, so if I can help, I would be glad.
Thanks

@danilogsch
Copy link
Contributor Author

Hi, is there anything I can do to help here? I have been waiting for this to be merged for a while, so if I can help, I would be glad. Thanks

Hey, thanks for asking... could you help me with the tests? I've been using Garden in my research, I did not have time to look into the DiffDriveOdometry_TEST.cc and to mimic that just yet

danilogsch and others added 8 commits June 1, 2023 20:48
line length corrections
Version v2 of the actions/checkout workflow is
deprecated, so switch to v3.

Part of gazebo-tooling/release-tools#862.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
The LICENSE file contained a copy of the stanze
used at the top of source code files, while the
actual license was in the COPYING file. So remove
the stanza and put the actual Apache 2.0 license text
in LICENSE.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
* Custom PID error rate

Signed-off-by: Nate Koenig <natekoenig@gmail.com>

* added test

Signed-off-by: Nate Koenig <natekoenig@gmail.com>

---------

Signed-off-by: Nate Koenig <natekoenig@gmail.com>
Signed-off-by: Michael Carroll <michael@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
@muttistefano
Copy link
Contributor

ok,i can take a look into that.
I have a question regarding the pull, I see it targets ign-math6, this means that after that we will have to do another one into the gz-math7 branch ?
Thanks

@Bi0T1N
Copy link
Contributor

Bi0T1N commented Jun 2, 2023

I have a question regarding the pull, I see it targets ign-math6, this means that after that we will have to do another one into the gz-math7 branch ? Thanks

Yes, as ign-math6 is only for Citadel and Fortress. The merge into gz-math7 should be quite simple as it doesn't touch many other files but it should be rebased on the current HEAD due to the ignition to gz renaming.

@danilogsch
Copy link
Contributor Author

ok,i can take a look into that. I have a question regarding the pull, I see it targets ign-math6, this means that after that we will have to do another one into the gz-math7 branch ? Thanks

I made this two branches when Garden was released:
gz-sim7
gz-math7

basic the same as this PR, feel free to use it

@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 31, 2023
mjcarroll and others added 2 commits August 15, 2023 20:20
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
Co-authored-by: muttistefano <mutti.stefano.jp@gmail.com>
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll
Copy link
Contributor

I just addressed the linting issues here and pulled in @muttistefano's test from #537

Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't verified with gz-sim, but I've looked at the equations and the referenced website and it all looks good to me. I'd say the assumption of a 1.0 wheel radius should be explicitly stated in the class header and in the gz-sim system that uses this.

include/gz/math/MecanumDriveOdometry.hh Outdated Show resolved Hide resolved
include/gz/math/MecanumDriveOdometry.hh Show resolved Hide resolved
Signed-off-by: Michael Carroll <mjcarroll@intrinsic.ai>
@mjcarroll mjcarroll merged commit 469540c into gazebosim:ign-math6 Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏰 citadel Ignition Citadel enhancement New feature or request 🏯 fortress Ignition Fortress
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants